chore(performance): optimize JSON parsing in get_actions and snapshot reading #3830
Conversation
fvaleye
force-pushed
the
performance/json-parsing
branch
from
4c40147 to
994ffca
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3830 +/- ##
==========================================
- Coverage 74.30% 74.29% -0.01%
==========================================
Files 147 147
Lines 39701 39695 -6
Branches 39701 39695 -6
==========================================
- Hits 29500 29493 -7
- Misses 8810 8812 +2
+ Partials 1391 1390 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… reading Signed-off-by: Florian Valeye <[email protected]>
fvaleye
force-pushed
the
performance/json-parsing
branch
from
994ffca to
d50077b
Compare
| pub async fn get_actions( | ||
| version: i64, | ||
| commit_log_bytes: bytes::Bytes, | ||
| commit_log_bytes: &bytes::Bytes, | ||
| ) -> Result<Vec<Action>, DeltaTableError> { |
There was a problem hiding this comment.
I wonder why this function is async. There's nothing async inside of it. Not your fault as the base function was also async, but probably some legacy tech debt? I can imagine there was a world where this function took an async bytes stream instead of all the bytes.
There was a problem hiding this comment.
per git blame, this function was implemented two years ago as async even though there were no async in it at any time.
There was a problem hiding this comment.
more generally speaking, I see most call sites disappearing short term, since log replay nor produces record batches that we extract data from (i.e. LogFileView et. al.) avoiding copies whenver possible.
Exception being calling this in commit_infos.
Sine we are passing in Bytes, I see no reason why we should be doing IO in this function, and with that also little reason for it to be async ... maybe in a follow-up we can make it sync.
There was a problem hiding this comment.
Also, let's see when we have fully kernelized conflict resolution. There might be a few surprises lurking 😆.
There was a problem hiding this comment.
Also, let's see when we have fully kernelized conflict resolution. There might be a few surprises lurking 😆.
Yay!
Let's keep it like this for now and make it sync later.
I will create an issue for tracking this need.
4 participants
Description
Following this PR, I took more time to investigate how to optimize the JSON parsing in
get_actions().I added a bench to evaluate the performance gains by comparing the baseline to the new implementation (leveraging
Deserializer::from_slicefrom serde_json).Benchmark results
Only for the
get_actions()methodMy hardware: Mac Book Pro M1
The performance boost of
13-27%depending on JSON complexityPerformance Results (150 samples, 10s measurement)
Simple Actions (1000 add actions)
Baseline: ~360-398 µs (2.51-2.77 Melem/s)
New version: ~311-312 µs (3.20-3.21 Melem/s)
Improvement:
~17-18%fasterWith Stats (1000 actions with stats)
Baseline: ~940 µs (1.06 Melem/s)
New version: ~688-717 µs (1.39-1.45 Melem/s)
Improvement:
~26-27%fasterFull Complexity (1000 complex actions)
Baseline: ~1.41 ms (708-710 Kelem/s)
New version: ~1.22 ms (817-819 Kelem/s)
Improvement:
~13-15%fasterChanges
Deserializer::from_slice()&bytes::Bytesavoids atomic ref-counting overheadSide notes: I will remove the
get_actions_baseline(), but we could keep the bench test